-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Chat - File does not appear with strikethrough style when uploaded offline and deleted #47971
base: main
Are you sure you want to change the base?
Conversation
…ed offline and deleted. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@dubielzyk-expensify @mananjadhav One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Let me know when there's something visual to check out :) |
The code changes are done, I will test last time today and upload the screenshots. |
@dubielzyk-expensify, please check the recording and let me know if we want some changes. Monosnap.screencast.2024-08-28.05-29-20.mp4 |
Yup, I'm also curious about what Jon is mentioning. |
Same. Other than that weirdness Jon pointed out I think it's looking good. |
Thanks for the review, I think that was already deleted action which might have failed that's why it was shown but I will double check that. |
@Expensify/design, I apologize for forgetting to show the visuals in light mode 😅. Could you please confirm if the video looks correct in both modes? Monosnap.screencast.2024-08-29.06-23-50.mp4 |
Thanks for providing video for light mode 😄 Is the icon using the icon color variable? Cause it looks to be the same icon color as in dark mode which is a darker than we want. In light mode it should appear a bit lighter. But it could be the recording screwing up with me here. |
I think the video might be correct? I think the color for icons is darker in light mode, and lighter in dark mode. |
Thanks, I will update and provide the result shortly. |
@Expensify/design, does this look correct? The color code has been updated to the deleted_indicator_updated_color.mp4 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Yeah I definitely want to just use the variables as they are.
Yeah I agree, though I think the whole entire post gets reduced opacity (even the name/avatar/timestamp) so I think if we're using the right variables I'm fine with how this is looking. |
SGTM. Let's push ahead unless @shawnborton thinks otherwise |
Yup same, all good with me! |
Thanks everyone, will update the recordings for all platforms soon, then this will be ready for C+ review. |
@mananjadhav, I have updated the test steps and added the videos for all platforms except for android native. There is a android specific bug I'm trying to solve. The trash icon opacity is also reduced. Let me know if you have an idea about this issue. Thanks Monosnap.screencast.2024-09-04.13-32-54.mp4 |
@mananjadhav, can you please take a look at the comment above? |
I'll be able to take a look at this tomorrow. |
@mananjadhav, can you please have a look? I think we are super close to merging this, the implementation works well on all platforms except android native :( |
I couldn't find the solution for the Android bug. I am still working on this. |
@mananjadhav, I think we should open a new issue for the opacity bug, as I believe it's out of the scope of this PR. We should try to close this one as soon as possible since it's been open for almost a month. But I will try again to fix the opacity bug today or tomorrow. |
yeah let's do that. @neil-marcellini does that work? @Krishna2323 can you please merge conflicts? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
merged main, I'm not sure about the ESlint check, I think that's not from this PR. |
@Krishna2323 I think we'll need to fix the eslint issues even if they're unrelated to the PR. It is because we're touching those files and the new lint rules are failing. @neil-marcellini can you please confirm? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments.
src/components/HTMLEngineProvider/HTMLRenderers/VideoRenderer.tsx
Outdated
Show resolved
Hide resolved
src/components/Attachments/AttachmentView/DefaultAttachmentView/index.tsx
Outdated
Show resolved
Hide resolved
return ( | ||
<> | ||
<View | ||
style={[styles.pAbsolute, styles.alignItemsCenter, styles.justifyContentCenter, styles.highlightBG, styles.deletedIndicatorOverlay, styles.deletedIndicator, containerStyles]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still going through these styles.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Hi, thanks for asking. The latest I've heard is that if the ES Lint checks are related to files touched in the current PR, then we require them to be fixed before the issue can be considered complete and paid. If the fixes are small then we like to see the changes in the current PR. Otherwise, like in this case where we need to migrate to useOnyx, we should handle it in a separate PR. You could also search to see if there is already an issue to migrate to useOnyx for these files. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@mananjadhav, I have fixed other files but I'm no sure what to do with App/src/components/HTMLEngineProvider/HTMLRenderers/ImageRenderer.tsx Lines 117 to 126 in 7dde09e
|
@mananjadhav, friendly bump on the comment above ^ |
@mananjadhav, any updates? |
I couldn't check this over the weekend, but I'll try to check today. I think it's best to post on open-source channel to get more eyes. |
@mananjadhav, we no longer need to make this change, I guess it has been done somewhere else. You can continue with the checklist. |
Details
Fixed Issues
$ #46616
PROPOSAL: #46616 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Monosnap.screencast.2024-09-04.13-32-54.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.1.mp4
MacOS: Desktop
desktop_app.mp4